-
Notifications
You must be signed in to change notification settings - Fork 112
Add API rate limit handler #371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add API rate limit handler #371
Conversation
src/neo4j_graphrag/llm/rate_limit.py
Outdated
raise convert_to_rate_limit_error(e) | ||
raise | ||
|
||
return active_handler.handle_sync(inner_func)() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to call the returned function here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not so familiar with defining decorators. But don't we still need this to return the retry version of the inner function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decorator should return a function, that's why I tend to think the final ()
are not needed, but I haven't tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we omit the trailing (), the decorator would return the wrapped function object instead of the function’s result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then shouldn't the return type of rate_limit_handler
by Any
instead of F
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but doesn't using F
preserve more type safety since at least at call sites the method has the same type as it has before decoration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would if we were returning a function, but based on the above discussion I understood that we're returning the result of the method? Also, what happens when removing the type ignore
comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inner function and the wrapper are returning the result of the original function, but the decorator is returning a function.
I am following the below pattern:
def my_decorator(func: Callable) -> Callable:
def wrapper(*args, **kwargs) -> Any:
def inner(): -> Any
print("Inner function logic")
return func(*args, **kwargs)
return inner()
return wrapper
my_decorator
: takes a functionfunc
and returns a new function (wrapper
)wrapper
: actual replacement forfunc
. It takes arguments, defines an inner function, and returns the result of inner().- inner: helper inside wrapper that encapsulates additional logic, then calls and returns the result of
func
.
obviously mypy will complain about incompatible return types, thus type ignore is added. Otherwise your suggestion of changing return type of the decorator to Any
will pass the mypy checks but the thing here is that the decorator could be misused afterwards.
I don't mind changing it, but I'd like to understand what's the best solution in this case or am I following the wrong pattern?
… provider classes
8cfacb4
to
af3a5d2
Compare
Just thinking as I was writing the release notes (sorry!): do we need to update this example as well to show how to use rate limiter for custom implementation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @NathalieCharbel, thank you!
Description
RateLimitHandler
interfaceRateLimitError
RateLimitHandler
interfaceType of Change
Complexity
Complexity: Medium
How Has This Been Tested?
Checklist
The following requirements should have been met (depending on the changes in the branch):